Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration API for record-based entities #1241

Merged
merged 63 commits into from
Mar 4, 2020
Merged

Conversation

dmitrykuzmin
Copy link
Contributor

@dmitrykuzmin dmitrykuzmin commented Feb 28, 2020

This PR extends the RecordBasedRepository with API to perform the migration of stored entities to account for the model changes.

Migration API

The repository is extended with applyMigration(ID, Migration) and applyMigration(Set<ID>, Migration) methods.

The Migration is a transformation operation which accepts entities one-by-one and modifies their state, lifecycle flags or can even delete the entity record. This class (or rather its descendants, ProjectionMigration and ProcessManagerMigration) can be inherited to create custom user-defined migration operations.

The applied entity modification can be configured by implementing the method apply(entityState):

class ProjectMigration extends ProjectionMigration<...> {

    // A mapper from the "old" to a "new" state.
    public Project apply(Project state) {
        return state.toBuilder()
                    .setName("new_name")
                    .build();
    }
}

The new state returned from apply will then be injected into the entity under migration.

In apply(...), it's also possible to call additional modification methods like markArchived() or removeFromStorage(). See Migration for details.

Each migration is preceded by the entity lookup by ID and is finalized by either saving the modified entity back to the repository or deleting the entity record, if it is configured by the Migration.

All changes are applied to entity under an active Transaction, with the last step of migration being a transaction commit. This means all entity lifecycle events are triggered normally, enabling subscription updates and other system-events-dependent features.

The posted events will have an instance of newly introduced MigrationApplied system event as a producing message.

Columns update

The standard migrations provided by Spine include the UpdateProjectionColumns and UpdatePmColumns migrations for projections and process managers respectively. When applied, such migration triggers the recalculation of interface-based columns of an entity, propagating the new values to the storage fields as well as to the serialized entity state.

The column update can be applied as follows:

// Projection.
projectionRepository.applyMigration(ImmutableSet.of(id1, id2), new UpdateProjectionColumns<>());

// Process manager.
pmRepository.applyMigration(ImmutableSet.of(id1, id2), new UpdatePmColumns<>());

Other migrations

The framework also provides Mark(Projection/Pm)Archived, Mark(Projection/Pm)Deleted and Remove(Projection/Pm)FromStorage migrations which can mark entity archived, mark entity deleted and physically remove an entity record from the storage respectively. The migrations are available for both projections and process managers under the respective migration packages.

Other

This PR also fixes an issue with Nullable interface-based columns propagation and improves the description of Columns.valuesIn(...) operation.

Version

Spine version advances to 1.4.11.

yuri-sergiichuk and others added 30 commits February 20, 2020 13:09
…into fix-interface-based-columns

� Conflicts:
�	license-report.md
An issue described by the TODO is rather serious and requires further discussion and probably changes of several public and internal API elements. So it's better off being converted into a GitHub issue.
@dmitrykuzmin
Copy link
Contributor Author

@armiol, @yuri-sergiichuk PTAL.

import io.spine.server.projection.Projection;
import io.spine.server.projection.ProjectionMigration;

public final class Migrations {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say these migrations could be part of the Framework standard migrations. Worth considering moving them to the sources?

Also documented the method better.
The `MarkArchived`, `MarkDeleted` and `RemoveFromStorage` migrations are now exposed as standard Spine migrations for both projections and process managers.

`ProjectionColumnsUpdate` and `ProcessManagerColumnsUpdate` renamed to `UpdateColumns`.
@dmitrykuzmin
Copy link
Contributor Author

@yuri-sergiichuk PTAL again.

* @return the event or an empty {@code Optional} if the posting was blocked by the
* {@link #eventFilter}
*/
public final Optional<Event> onMigrationApplied() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit strange that we're able to filter out the migration event, but on the other hand, in the Migration we throw an IllegalStateException in case of such filtering.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmitrykuzmin LGTM in general. Please see my suggestions though.

* {@link MigrationApplied} event as the producing message.
*
* <p>To create a user-defined {@code Migration} in real life scenarios, consider inheriting from
* {@link io.spine.server.projection.ProjectionMigration} and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use an alt text to shorten the links. E.g. {@link io.spine.server.projection.ProjectionMigration ProjectionMigration}.

* A migration operation that does the update of interface-based columns of a
* {@link ProcessManager}.
*
* <p>When applied to an entity, this operation will trigger the recalculation of entity storage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please append a paragraph explaining when such migration may be handy. I.e. describe a use-case scenario.

/**
* A migration operation that does the update of interface-based columns of a {@link Projection}.
*
* <p>When applied to an entity, this operation will trigger the recalculation of entity storage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

* @see io.spine.server.entity.RecordBasedRepository#applyMigration(Object, Migration)
*/
@Experimental
public final class UpdateColumns<I,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a single UpdateColumns class, which would be created using a factory method?

E.g. UpdateColumns.of(MyProjectView.class) or UpdateColumns.of(MyProcess.class).

We then would detect if it is a projection or a process manager and create a corresponding instance.

It would be cool to avoid two public classes with the same name.

Now in such case the framework will just log a warning.
The old standard migrations are now renamed to preserve naming uniqueness (e.g. `procman.MarkArchived` -> `MarkPmArchived`) but are also made `Internal`.
The approach with delegating migrations doesn't seem to work unless we expose even more `Migration` methods as `protected` `Internal` which is undesirable.
Copy link
Contributor

@yuri-sergiichuk yuri-sergiichuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The `EntityDeleted` event message is extended with `deletion` one-of which allows to determine whether the entity was just marked as deleted or physically removed from the storage.
…into extend-migration-api

� Conflicts:
�	license-report.md
�	pom.xml
�	version.gradle
@dmitrykuzmin
Copy link
Contributor Author

@armiol PTAL again.

@dmitrykuzmin dmitrykuzmin requested a review from armiol March 4, 2020 14:07
@dmitrykuzmin dmitrykuzmin merged commit 93040b9 into master Mar 4, 2020
@dmitrykuzmin dmitrykuzmin deleted the extend-migration-api branch March 4, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants